-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support @deprecate
ing qualified function names
#44394
Conversation
@fredrikekre Since you just reviewed #44388 (thanks!) and already tweaked this part of the code, do you mind reviewing this as well? |
Is this just so you don't have to put the macro call in the module A
end
bar() = 1
@deprecate A.foo bar |
Yeah, the callable object should already exist in that namespace. It'd fail otherwise. One difference to the version before this PR is that we can emit the message with the name that would be more likely to be used in practice. For example, with this PR, I get julia> @deprecate Threads.Atomic() Threads.Atomic{Int}() false
julia> Threads.Atomic()
┌ Warning: `Threads.Atomic()` is deprecated, use `Threads.Atomic{Int}()` instead.
│ caller = ip:0x0
└ @ Core :-1 while before this PR it would be julia> import Base.Threads: Atomic
@deprecate Atomic() Atomic{Int}() false;
julia> Threads.Atomic()
┌ Warning: `Atomic()` is deprecated, use `Atomic{Int}()` instead.
│ caller = ip:0x0
└ @ Core :-1 Notice that the warning message with this PR shows |
Okay, but if you would have put it in the correct module it would work even if the name didn't exist. It doesn't seem too much hassle to put deprecations in the module they belong? I don't have any strong feelings about the PR, just wondering whether it is useful enough since it will only work in pretty specific cases.
But that already works (won't print the module on the old usage, but it displays what you should update too at least): julia> @eval Base.Threads @deprecate Atomic() Threads.Atomic{Int}() false;
julia> Threads.Atomic()
┌ Warning: `Atomic()` is deprecated, use `Threads.Atomic{Int}()` instead. |
I think it makes sense to be cautious if this PR were adding code to achieve what I wanted it for. But it actually just simply removes an artificial restriction that the function names have to be a symbol. For example, this PR also lets us use something like |
@deprecate
ing functions in other modules@deprecate
ing qualified function names
Okay, that makes sense I guess. Does @deprecate A.foo A.bar also work? I didn't see any changes wrt that. Could probably be included in the first code path ( |
I just pass through all non-call expressions ATM. But it probably doesn't do what we want it to do julia> struct Foo{T}
value::T
end
julia> (Foo{T} where {T <: Int})(x) = x # this happens with `@deprecate Foo{T} where {T <: Int} ...`
julia> Foo{Int}(0)
Foo{Int64}(0)
julia> (Foo{T}(x) where {T <: Int}) = x
julia> Foo{Int}(0)
0 Maybe we need to restrict the non-call form to symbols and getfields? |
Actually, the scenario I worried about was already detected in the current code. I just added a test to check it. It's good to go on my end. Are there any other concerns? |
This PR adds support for code like this
#44388 needs to go first